support draining multiple node #469
Conversation
refactor the drainNode function to support drainning multiple node Signed-off-by: Andrew Hu <[email protected]>
andrewhu-hcl
requested review from
ispeakc0de,
ksatchit and
uditgaurav
as code owners
|
|
||
| common.SetTargets(experimentsDetails.TargetNode, "injected", "node", chaosDetails) | ||
| log.Infof("Target nodes list: %v", targetNodes) | ||
| for _, targetNode := range targetNodes { |
There was a problem hiding this comment.
Can we also handle the panic case when the length of the list is zero and give an error message to provide the target node name.
There was a problem hiding this comment.
When we run chaos for more nodes we could perform the pre and post chaos check for all the target nodes.
There was a problem hiding this comment.
We can also change experimentsDetails.TargetNode to experimentsDetails.TargetNodes in the structure and other places where it is used.
Signed-off-by: Caleb Xu <[email protected]>
Signed-off-by: Caleb Xu <[email protected]>
Signed-off-by: Caleb Xu <[email protected]>
calebxu-hcl
force-pushed
the
support-multiple-node-drain
branch
from
bfa3c9b
to
05726d3
Compare
|
Thanks for the feedback @uditgaurav, I think I've addressed all three items you mentioned above. Please advise if there's still any concerns. |
Signed-off-by: Caleb Xu <[email protected]>
calebxu-hcl
force-pushed
the
support-multiple-node-drain
branch
from
916934b
to
96a94ec
Compare
|
Thanks, @andrewhu-hcl for being patient! Can we also add the logs of experiments with successful execution for multiple nodes? |
| default: | ||
| log.Infof("[Inject]: Draining the %v node", experimentsDetails.TargetNode) | ||
| targetNodes := strings.Split(experimentsDetails.TargetNodes, ",") | ||
| if len(targetNodes) == 0 { |
There was a problem hiding this comment.
Even for an empty string value in experimentsDetails.TargetNodes, the len(targetNodes) will be equal to 1. We can modify the check as follows:
| if len(targetNodes) == 0 { | |
| if experimentsDetails.TargetNodes == "" { |
| command.Stderr = &stderr | ||
| if err := command.Run(); err != nil { | ||
| log.Infof("Error String: %v", stderr.String()) | ||
| return errors.Errorf("Unable to drain the %v node, err: %v", targetNode, err) |
There was a problem hiding this comment.
We can pass the stderr here since the err will contain the exit code description e.g. exit code 1.
| nodeSpec, err := clients.KubeClient.CoreV1().Nodes().Get(targetNode, v1.GetOptions{}) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil |
There was a problem hiding this comment.
Shall we add a log here to specify that the resource was not found?
| @@ -183,7 +203,11 @@ func uncordonNode(experimentsDetails *experimentTypes.ExperimentDetails, clients | |||
| Times(uint(experimentsDetails.Timeout / experimentsDetails.Delay)). | |||
| Wait(time.Duration(experimentsDetails.Delay) * time.Second). | |||
| Try(func(attempt uint) error { | |||
| targetNodes := strings.Split(experimentsDetails.TargetNode, ",") | |||
| targetNodes := strings.Split(experimentsDetails.TargetNodes, ",") | |||
| if len(targetNodes) == 0 { | |||
There was a problem hiding this comment.
Same comment as above, this check will fail for an empty string.
refactor the drainNode function to support draining multiple node
Signed-off-by: Andrew Hu [email protected]
What this PR does / why we need it:
Current node drain experiment, if the targetNodes is provided, it will get all the node names by split the comma and check each node status. However, during the node drain stage, it just passed the targetNodes into the node drain command, didn’t check if a string contained multiple nodes with comma-separated. So it will cause an error if we specify the targetNodes as multiple node name list by comma-separated. It will be great if it can support draining the multiple nodes.
Which issue this PR fixes: fixes #3343
Special notes for your reviewer:
Checklist:
breaking-changestagrequires-upgradetag